Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change TLS MinVersion to tls.VersionTLS12 in order to make Triggers run on OCP where FIPS enabled #1518

Merged
merged 1 commit into from
Feb 8, 2023
Merged

Change TLS MinVersion to tls.VersionTLS12 in order to make Triggers run on OCP where FIPS enabled #1518

merged 1 commit into from
Feb 8, 2023

Conversation

savitaashture
Copy link
Contributor

@savitaashture savitaashture commented Feb 1, 2023 •
edited
Loading

Changes

  • Fixes: tls: client offered only unsupported versions #1463 this issue occurs on Openshift cluster where fips are enabled

    Issue: http: TLS handshake error from 10.128.2.1:36306: tls: client offered only unsupported versions: [303]
    RCA:

    1. During peer-peer https communication, when TLS handshake is happening, at a stage where both server and client mutually agree for a common TLS version and cipher,, The Triggers Interceptor server is enabled or only allow TLS 1.3 and corresponding ciphers today, where the client(OCP) giving a TLS 1.2 tls version and cipher(Based on error http: TLS handshake error from 10.128.2.1:36306: tls: client offered only unsupported versions: [303] Ref,)
      which result in the TLS handshake not happening as there is no mutual TLS version to negotiate from both (server and client ) side

    2. OCP uses MInTLS as 1.2 for all components

    Fix:

    1. Changed MinVersion to tls.VersionTLS12 and it works on both K8S and OCP

  • Moved config/interceptors/interceptor-secrets.yaml to config/interceptors/00-interceptor-secrets.yaml so that secrets will be created before core-intercetor pod start this solves issue like tekton-triggers-core-interceptors-certs not found

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Changed TLS MinVersion to `tls.VersionTLS12` in order to make Triggers run on OCP(Where FIPS enabled) as OCP uses MInTLS as 1.2 for all components

@savitaashture savitaashture requested review from khrm and dibyom February 1, 2023 16:38
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 1, 2023
@tekton-robot tekton-robot requested a review from dlorenc February 1, 2023 16:38
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 1, 2023
@savitaashture
Copy link
Contributor Author

@dibyom @khrm we may need v0.22.2 patch release for this fix as its blocking to use Triggers on a cluster where fips are enabled

khrm
khrm reviewed Feb 1, 2023
View reviewed changes
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2023
@khrm
Copy link
Contributor

khrm commented Feb 1, 2023

@savitaashture Removing min version in the tls, is flagged as a security issue by gosec. Does it really resolve FIPS?

@dibyom
Copy link
Member

dibyom commented Feb 1, 2023

@savitaashture Removing min version in the tls, is flagged as a security issue by gosec. Does it really resolve FIPS?

+1. Don't know a lot about FIPS but from https://developers.redhat.com/articles/2022/05/31/your-go-application-fips-compliant it sounds like you have to build with a different set of crypto libraries for it to be FIPS compatible?

@savitaashture
Copy link
Contributor Author

savitaashture commented Feb 2, 2023 •
edited
Loading

@savitaashture Removing min version in the tls, is flagged as a security issue by gosec. Does it really resolve FIPS?

+1. Don't know a lot about FIPS but from https://developers.redhat.com/articles/2022/05/31/your-go-application-fips-compliant it sounds like you have to build with a different set of crypto libraries for it to be FIPS compatible?

@khrm @dibyom Yes agree

Looked deeper and discussed with those who know about FIPS

Reason:

  1. During peer-peer https communication, when TLS handshake is happening, at a stage where both server and client mutually agree for a common TLS version and cipher,, The Triggers Interceptor server is enabled or only allow TLS 1.3 and corresponding ciphers today, where the client(OCP) giving a TLS 1.2 tls version and cipher(Based on error http: TLS handshake error from 10.128.2.1:36306: tls: client offered only unsupported versions: [303] Ref,)
    which result in the TLS handshake not happening as there is no mutual TLS version to negotiate from both (server and client ) side

  2. OCP uses MInTLS as 1.2 for all components

So now changed MinVersion to tls.VersionTLS12 and it works on both K8S and OCP

khrm
khrm reviewed Feb 2, 2023
View reviewed changes
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@savitaashture
Copy link
Contributor Author

@dibyom @khrm we may need v0.22.2 patch release for this fix as its blocking to use Triggers on a cluster where fips are enabled

We may need to do v0.21.x because this issue exist in v0.21.x as well

@dibyom
Copy link
Member

dibyom commented Feb 2, 2023

Thanks @savitaashture - the explanation is very helpful. Can we update the PR description/commit message with this?
/lgtm
/hold
(feel free to cancel the hold when you have updated!)

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2023
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2023
@savitaashture savitaashture changed the title Fix tls: client offered only unsupported versions Change TLS MinVersion to tls.VersionTLS12 in order to make Triggers run on OCP where FIPS enabled Feb 3, 2023
@savitaashture
Copy link
Contributor Author

Thanks @savitaashture - the explanation is very helpful. Can we update the PR description/commit message with this? /lgtm /hold (feel free to cancel the hold when you have updated!)

Thank you @dibyom

Updated commit message and Description

@savitaashture savitaashture removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2023
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

2 similar comments
@khrm
Copy link
Contributor

khrm commented Feb 6, 2023

/test pull-tekton-triggers-integration-tests

@khrm
Copy link
Contributor

khrm commented Feb 8, 2023

/test pull-tekton-triggers-integration-tests

@savitaashture savitaashture added this to the Triggers v0.23 milestone Feb 8, 2023
@tekton-robot tekton-robot merged commit 7c0650a into tektoncd:main Feb 8, 2023
khrm
khrm reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khrm khrm khrm left review comments

@dibyom dibyom Awaiting requested review from dibyom

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees

@dibyom dibyom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
Triggers v0.23
Development

Successfully merging this pull request may close these issues.
4 participants
@savitaashture @khrm @dibyom @tekton-robot